fix(langchain): langchain end span#3007
Conversation
|
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 0993186 in 1 minute and 32 seconds. Click for details.
- Reviewed
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py:397
- Draft comment:
Consider usingself.spans.pop(run_id, None)for removing the span associated with run_id. This approach is more concise and reduces the chance of errors. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_RwfZg1GoT5gbkyj1
You can customize 
|
Hey @DRXD1000 thanks for opening this! I'm happy to jump into the investigation on
Do you have any minimal repro application and setup for tracking memory consumption? |
|
@dinmukhamedm sadly not. I tested with a running application using the memray package locally and grafana kubernetes Dashboards. The application i tested with is a langgraph stategraph inside a fastapi application. Maybe someone from the mentioned issue can provide a minimal example. What i can provide right now are screenshots of the memray graphs. The only thing i changed is the openllmetry package. Both runs have gone trough 120 requests to the fastapi service. Openllmetry main branch without modification 512MB -> 543.3 MB
Branch of this pull request 455.9MB -> 458.3MB
|
nirga
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Child Span Cleanup Fails
The memory leak fix for child spans is incomplete. The self.spans.pop(child_id, None) call is incorrectly placed inside the if child_span.end_time is None: condition. This prevents child spans that were already ended from being removed from self.spans, causing a persistent memory leak for those spans. To fully address the leak, self.spans.pop(child_id, None) should be moved outside this condition to ensure all child spans are cleaned up regardless of their end status.
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py#L397-L401
BugBot free trial expires on July 22, 2025
You have used $0.00 of your $50.00 spend limit so far. Manage your spend limit in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
nirga
dismissed
their stale review
Interesting - this seems to cause some of the tests to fail, and it seems some spans are not being generated no because of this change. Can it be that the order of _end_span is not what we expect and so we may need to access a certain span or their children after _end_span has been called? 🤔
|
Thanks @nirga for checking in. I would really appreciate if you could investigate this :) |
|
Yes will do too! @DRXD1000 |
|
We would greatly appreciate this fix and would be very happy if this PR could be merged soon! Thank you so much 😃 |
|
@nirga Any updates on this ? |
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I think #3216 should fix this - closing this in favor of my PR. |
5 participants
feat(instrumentation): ...orfix(instrumentation): ....Regarding #2790 i have experimented with this change. As far as i have seen it in my applications, this reduces the memory leak significantly, but does not fix it completely.
I would really much appreciate if someone could look at this change and check its usability.
Important
Fixes memory leak in
TraceloopCallbackHandlerby ensuring spans are deleted fromself.spansafter ending.TraceloopCallbackHandlerby deleting spans fromself.spansafter they are ended in_end_span().child_idandrun_idfromself.spansif they exist.This description was created by
for 0993186. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit